test: add concurrency testing for storage operations #1132

josecorella · 2024-12-17T00:03:47Z

Issue #, if available:

Description of changes:

Squash/merge commit message, if applicable:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

...ware/amazon/cryptography/example/hierarchy/concurrent/ConcurrentConditionCheckWriteTest.java

texastony · 2024-12-17T01:36:30Z

...va/src/testExamples/java/software/amazon/cryptography/example/hierarchy/concurrent/README.md

What is this doing here?
Is this temporary?

it is temporary. I wanted to add a barebones README for these tests

github-actions · 2024-12-17T01:37:21Z

@josecorella and @texastony, I noticed you are updating the smithy model files.
Does this update need new or updated user documentation?
Are you adding constraints inside list, map or union? Do you know about this issue: smithy-lang/smithy-dafny#491?

josecorella · 2024-12-17T21:53:00Z

force pushed due to rebase from mutations/mutations

texastony

This is Awesome!
But there are some things I think we need to fix up before calling this done.

Also, right now, I cannot figure out if the new library_concurrency_tests.yml was run by GitHub CI here or not.

145 checks passed.
mutations/mutations has 142 checks.

So... maybe the new checks ran?
But I cannot figure out which of the 145 checks they are,
I think because their name is Library Interoperability Tests which conflicts with another GitHub workflow.

texastony · 2024-12-18T18:50:59Z

.github/workflows/library_concurrency_tests.yml

+# This workflow performs interoperability tests across the supported runtimes of the MPL.
+name: Library Interoperability Tests


Blocking: We need to fix this up.
Or else the Job will run under the wrong name.

texastony · 2024-12-18T18:52:30Z

.github/workflows/library_concurrency_tests.yml

+          ]
+        # https://taskei.amazon.dev/tasks/CrypTool-5284
+        dotnet-version: ["6.0.x"]
+        java-versions: [8, 11, 16, 17]


non-blocking/comment: For my money, I would only test on Java 8 and Java 17.
Maybe even only Java 8.

I do not think the subject of these tests changes behavior under different Java versions.

texastony · 2024-12-18T18:53:03Z

.github/workflows/library_concurrency_tests.yml

+        # https://taskei.amazon.dev/tasks/CrypTool-5284
+        dotnet-version: ["6.0.x"]
+        java-versions: [8, 11, 16, 17]
+    runs-on: ${{ matrix.os }}


non-blocking/question: Does the subject of these tests change behavior across OS-es?

nope, copy-pasta

texastony · 2024-12-18T18:53:35Z

.github/workflows/library_concurrency_tests.yml

+        with:
+          aws-region: us-west-2
+          role-to-assume: arn:aws:iam::370957321024:role/GitHub-CI-MPL-Dafny-Role-us-west-2
+          role-session-name: InterOpTests


Blocking: not InterOpTests but Concurrency tests, right?

yup! just copy pasta-ying

texastony · 2024-12-18T18:54:42Z

AwsCryptographicMaterialProviders/runtimes/java/build.gradle.kts

+}
+
+val testConcurrentExamples = task<Test>("testConcurrentExamples") {
+    description = "Runs examples tests."


Suggested change

description = "Runs examples tests."

description = "Runs Concurrency tests."

texastony · 2024-12-18T21:21:35Z

...are/amazon/cryptography/example/hierarchy/concurrent/StorageVersionReadConcurrencyTests.java

+          Assert.assertTrue(
+            (cancellationReason.code().equals("TransactionConflict") ||
+              cancellationReason.code().equals("None") ||
+              cancellationReason.code().equals("ConditionalCheckFailed"))


This assert only fires if an Exception is thrown.

There is no assert on the non-catch path.

Also, there is no evidence if, in the 10 * 100 = 1000 invocations,
one of these asserts was met.

texastony · 2024-12-18T21:23:07Z

...are/amazon/cryptography/example/hierarchy/concurrent/StorageVersionReadConcurrencyTests.java

+  @Test(
+    dependsOnMethods = {
+      "testConcurrentActiveReadWhileVersionInFlight",
+      "testConcurrentVersionWithStorage",
+    }
+  )
+  public void testVersionReads() {
+    Assert.assertFalse(versionKeyOutputMap.isEmpty());
+    for (String key : versionKeyOutputMap.keySet()) {
+      System.out.println(
+        "key: " + key + " value " + versionKeyOutputMap.get(key)
+      );
+    }
+  }


This method asserts that testConcurrentActiveReadWhileVersionInFlight succeeded at least once.

Yes. That is what I want. This test proves that while a versioning is going on we can still read.

texastony · 2024-12-18T21:25:02Z

...tware/amazon/cryptography/example/hierarchy/concurrent/StorageWriteReadConcurrencyTests.java

+    GetItemResponse res = DdbHelper.getKeyStoreDdbItem(
+      branchKeyId,
+      "branch:ACTIVE",
+      Fixtures.TEST_KEYSTORE_NAME,
+      DynamoDbClient.create()
+    );
+    DdbHelper.deleteKeyStoreDdbItem(
+      branchKeyId,
+      "branch:ACTIVE",
+      Fixtures.TEST_KEYSTORE_NAME,
+      DynamoDbClient.create(),
+      true
+    );
+    DdbHelper.deleteKeyStoreDdbItem(
+      branchKeyId,
+      "beacon:ACTIVE",
+      Fixtures.TEST_KEYSTORE_NAME,
+      DynamoDbClient.create(),
+      true
+    );
+    DdbHelper.deleteKeyStoreDdbItem(
+      branchKeyId,
+      res.item().get("version").s(),
+      Fixtures.TEST_KEYSTORE_NAME,
+      DynamoDbClient.create(),
+      true
+    );


Same as previous class, use DeleteBranchKey.

texastony · 2024-12-18T21:25:50Z

...tExamples/java/software/amazon/cryptography/example/hierarchy/concurrent/testng-parallel.xml

Awesome. Do we need to do something to ensure that both tests run?

yes, it's the gradle task
https://github.com/aws/aws-cryptographic-material-providers-library/pull/1132/files/c6b7b10939489f1cb48c8e8ee1ac7d38ad59be8f#diff-d871726274402e1846d6b6600e590ef2e4750ba8c33f33fd09af2937c94ecbb1R331-R333

texastony · 2024-12-18T21:27:00Z

AwsCryptographicMaterialProviders/runtimes/java/build.gradle.kts

+    testLogging {
+        events("passed")
+    }


If you are running 1000 + 150 + more tests, you may not want to log every pass.

these concurrency tests are expensive and I don't want them to run every pr - if i have them run once a week, I don't see an issue in logging the passes.

josecorella temporarily deployed to Duvet CI December 17, 2024 00:03 — with GitHub Actions Inactive

texastony reviewed Dec 17, 2024

View reviewed changes

texastony force-pushed the mutations/mutations branch from 8032a45 to 4b2b629 Compare December 17, 2024 16:41

josecorella added 5 commits December 17, 2024 13:52

test: add concurrency testing framework

47a6ed6

finish concurrency tests

49054c2

chore: add versioning concurrent tests and add parallelization

0e43aff

update readme

34f9190

feedback

b96c516

josecorella force-pushed the test/concurrent-operations-storage branch from a9c2675 to b96c516 Compare December 17, 2024 21:52

josecorella temporarily deployed to Duvet CI December 17, 2024 21:52 — with GitHub Actions Inactive

format

c6b7b10

josecorella temporarily deployed to Duvet CI December 17, 2024 23:00 — with GitHub Actions Inactive

josecorella marked this pull request as ready for review December 17, 2024 23:01

josecorella requested a review from a team as a code owner December 17, 2024 23:01

texastony requested changes Dec 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: add concurrency testing for storage operations #1132

test: add concurrency testing for storage operations #1132

josecorella commented Dec 17, 2024

texastony Dec 17, 2024

josecorella Dec 17, 2024

github-actions bot commented Dec 17, 2024

josecorella commented Dec 17, 2024

texastony left a comment •

edited

Loading

texastony Dec 18, 2024

texastony Dec 18, 2024

texastony Dec 18, 2024

josecorella Dec 18, 2024

texastony Dec 18, 2024

josecorella Dec 18, 2024 •

edited

Loading

texastony Dec 18, 2024

texastony Dec 18, 2024

texastony Dec 18, 2024

josecorella Dec 18, 2024

texastony Dec 18, 2024

texastony Dec 18, 2024

josecorella Dec 18, 2024

texastony Dec 18, 2024

josecorella Dec 18, 2024

		# This workflow performs interoperability tests across the supported runtimes of the MPL.
		name: Library Interoperability Tests

	description = "Runs examples tests."
	description = "Runs Concurrency tests."

test: add concurrency testing for storage operations #1132

Are you sure you want to change the base?

test: add concurrency testing for storage operations #1132

Conversation

josecorella commented Dec 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 17, 2024

josecorella commented Dec 17, 2024

texastony left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josecorella Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

texastony left a comment •

edited

Loading

josecorella Dec 18, 2024 •

edited

Loading